From: Keir Fraser Date: Wed, 4 Mar 2009 14:28:50 +0000 (+0000) Subject: Be careful with page_get_owner() now that owner field can be clobbered X-Git-Tag: archive/raspbian/4.8.0-1+rpi1~1^2~13995^2~6 X-Git-Url: https://dgit.raspbian.org/%22http://www.example.com/cgi/success//%22http:/www.example.com/cgi/success/?a=commitdiff_plain;h=ce870565b66bb8c09d4ac96dac49ae86672580e2;p=xen.git Be careful with page_get_owner() now that owner field can be clobbered by some users. Introduce get_page_owner_and_reference() where that can be more useful. Signed-off-by: Keir Fraser --- diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index cbbe2e0abc..006964498f 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -371,14 +371,14 @@ void share_xen_page_with_privileged_guests( #else /* * In debug builds we shadow a selection of <4GB PDPTs to exercise code paths. - * We cannot safely shadow the idle page table, nor shadow (v1) page tables - * (detected by lack of an owning domain). As required for correctness, we + * We cannot safely shadow the idle page table, nor shadow page tables + * (detected by zero reference count). As required for correctness, we * always shadow PDPTs above 4GB. */ -#define l3tab_needs_shadow(mfn) \ - (((((mfn) << PAGE_SHIFT) != __pa(idle_pg_table)) && \ - (page_get_owner(mfn_to_page(mfn)) != NULL) && \ - ((mfn) & 1)) || /* odd MFNs are shadowed */ \ +#define l3tab_needs_shadow(mfn) \ + (((((mfn) << PAGE_SHIFT) != __pa(idle_pg_table)) && \ + (mfn_to_page(mfn)->count_info & PGC_count_mask) && \ + ((mfn) & 1)) || /* odd MFNs are shadowed */ \ ((mfn) >= 0x100000)) #endif @@ -690,7 +690,16 @@ get_##level##_linear_pagetable( \ int is_iomem_page(unsigned long mfn) { - return (!mfn_valid(mfn) || (page_get_owner(mfn_to_page(mfn)) == dom_io)); + struct page_info *page; + + if ( !mfn_valid(mfn) ) + return 1; + + /* Caller must know that it is an iomem page, or a reference is held. */ + page = mfn_to_page(mfn); + ASSERT((page->count_info & PGC_count_mask) != 0); + + return (page_get_owner(page) == dom_io); } @@ -703,7 +712,6 @@ get_page_from_l1e( uint32_t l1f = l1e_get_flags(l1e); struct vcpu *curr = current; struct domain *owner; - int okay; if ( !(l1f & _PAGE_PRESENT) ) return 1; @@ -714,8 +722,13 @@ get_page_from_l1e( return 0; } - if ( is_iomem_page(mfn) ) + if ( !mfn_valid(mfn) || + (owner = page_get_owner_and_reference(page)) == dom_io ) { + /* Only needed the reference to confirm dom_io ownership. */ + if ( mfn_valid(mfn) ) + put_page(page); + /* DOMID_IO reverts to caller for privilege checks. */ if ( d == dom_io ) d = curr->domain; @@ -731,33 +744,29 @@ get_page_from_l1e( return 1; } + if ( owner == NULL ) + goto could_not_pin; + /* * Let privileged domains transfer the right to map their target * domain's pages. This is used to allow stub-domain pvfb export to dom0, * until pvfb supports granted mappings. At that time this minor hack * can go away. */ - owner = page_get_owner(page); - if ( unlikely(d != owner) && (owner != NULL) && - (d != curr->domain) && IS_PRIV_FOR(d, owner) ) + if ( unlikely(d != owner) && (d != curr->domain) && IS_PRIV_FOR(d, owner) ) d = owner; /* Foreign mappings into guests in shadow external mode don't * contribute to writeable mapping refcounts. (This allows the * qemu-dm helper process in dom0 to map the domain's memory without * messing up the count of "real" writable mappings.) */ - okay = get_data_page( - page, d, - (l1f & _PAGE_RW) && !(paging_mode_external(d) && (d != curr->domain))); - if ( !okay ) - { - MEM_LOG("Error getting mfn %lx (pfn %lx) from L1 entry %" PRIpte - " for dom%d", - mfn, get_gpfn_from_mfn(mfn), - l1e_get_intpte(l1e), d->domain_id); - } - else if ( pte_flags_to_cacheattr(l1f) != - ((page->count_info >> PGC_cacheattr_base) & 7) ) + if ( (l1f & _PAGE_RW) && + !(paging_mode_external(d) && (d != curr->domain)) && + !get_page_type(page, PGT_writable_page) ) + goto could_not_pin; + + if ( pte_flags_to_cacheattr(l1f) != + ((page->count_info >> PGC_cacheattr_base) & 7) ) { unsigned long x, nx, y = page->count_info; unsigned long cacheattr = pte_flags_to_cacheattr(l1f); @@ -786,7 +795,16 @@ get_page_from_l1e( #endif } - return okay; + return 1; + + could_not_pin: + MEM_LOG("Error getting mfn %lx (pfn %lx) from L1 entry %" PRIpte + " for dom%d", + mfn, get_gpfn_from_mfn(mfn), + l1e_get_intpte(l1e), d->domain_id); + if ( owner != NULL ) + put_page(page); + return 0; } @@ -1174,7 +1192,7 @@ static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e) for ( i = 0; i < PDPT_L2_ENTRIES; i++ ) { l2e = l2e_from_page( - virt_to_page(page_get_owner(page)->arch.mm_perdomain_pt) + i, + virt_to_page(d->arch.mm_perdomain_pt) + i, __PAGE_HYPERVISOR); l2e_write(&pl2e[l2_table_offset(PERDOMAIN_VIRT_START) + i], l2e); } @@ -1924,7 +1942,7 @@ void put_page(struct page_info *page) } -int get_page(struct page_info *page, struct domain *domain) +struct domain *page_get_owner_and_reference(struct page_info *page) { unsigned long x, y = page->count_info; @@ -1933,22 +1951,29 @@ int get_page(struct page_info *page, struct domain *domain) if ( unlikely((x & PGC_count_mask) == 0) || /* Not allocated? */ /* Keep one spare reference to be acquired by get_page_light(). */ unlikely(((x + 2) & PGC_count_mask) <= 1) ) /* Overflow? */ - goto fail; + return NULL; } while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x ); - if ( likely(page_get_owner(page) == domain) ) + return page_get_owner(page); +} + + +int get_page(struct page_info *page, struct domain *domain) +{ + struct domain *owner = page_get_owner_and_reference(page); + + if ( likely(owner == domain) ) return 1; put_page(page); - fail: if ( !_shadow_mode_refcounts(domain) && !domain->is_dying ) gdprintk(XENLOG_INFO, "Error pfn %lx: rd=%p, od=%p, caf=%08lx, taf=%" PRtype_info "\n", - page_to_mfn(page), domain, page_get_owner(page), - y, page->u.inuse.type_info); + page_to_mfn(page), domain, owner, + page->count_info, page->u.inuse.type_info); return 0; } @@ -1974,7 +1999,6 @@ static void get_page_light(struct page_info *page) while ( unlikely(y != x) ); } - static int alloc_page_type(struct page_info *page, unsigned long type, int preemptible) { diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 8a755ca347..1c32772a88 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -195,7 +195,7 @@ static void __gnttab_map_grant_ref( struct gnttab_map_grant_ref *op) { - struct domain *ld, *rd; + struct domain *ld, *rd, *owner; struct vcpu *led; int handle; unsigned long frame = 0, nr_gets = 0; @@ -336,8 +336,13 @@ __gnttab_map_grant_ref( spin_unlock(&rd->grant_table->lock); - if ( is_iomem_page(frame) ) + if ( !mfn_valid(frame) || + (owner = page_get_owner_and_reference(mfn_to_page(frame))) == dom_io ) { + /* Only needed the reference to confirm dom_io ownership. */ + if ( mfn_valid(frame) ) + put_page(mfn_to_page(frame)); + if ( !iomem_access_permitted(rd, frame, frame) ) { gdprintk(XENLOG_WARNING, @@ -352,20 +357,11 @@ __gnttab_map_grant_ref( if ( rc != GNTST_okay ) goto undo_out; } - else + else if ( owner == rd ) { - if ( unlikely(!mfn_valid(frame)) || - unlikely(!(gnttab_host_mapping_get_page_type(op, ld, rd) ? - get_page_and_type(mfn_to_page(frame), rd, - PGT_writable_page) : - get_page(mfn_to_page(frame), rd))) ) - { - if ( !rd->is_dying ) - gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n", - frame); - rc = GNTST_general_error; - goto undo_out; - } + if ( gnttab_host_mapping_get_page_type(op, ld, rd) && + !get_page_type(mfn_to_page(frame), PGT_writable_page) ) + goto could_not_pin; nr_gets++; if ( op->flags & GNTMAP_host_map ) @@ -383,6 +379,17 @@ __gnttab_map_grant_ref( } } } + else + { + could_not_pin: + if ( !rd->is_dying ) + gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n", + frame); + if ( owner != NULL ) + put_page(mfn_to_page(frame)); + rc = GNTST_general_error; + goto undo_out; + } if ( need_iommu(ld) && !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) && diff --git a/xen/common/xencomm.c b/xen/common/xencomm.c index c1e32bb0de..0bf00a7826 100644 --- a/xen/common/xencomm.c +++ b/xen/common/xencomm.c @@ -51,24 +51,16 @@ xencomm_get_page(unsigned long paddr, struct page_info **page) return -EFAULT; *page = maddr_to_page(maddr); - if ( get_page(*page, current->domain) == 0 ) + if ( !get_page(*page, current->domain) == 0 ) { - if ( page_get_owner(*page) != current->domain ) - { - /* - * This page might be a page granted by another domain, or - * this page is freed with decrease reservation hypercall at - * the same time. - */ - gdprintk(XENLOG_WARNING, - "bad page is passed. paddr 0x%lx maddr 0x%lx\n", - paddr, maddr); - return -EFAULT; - } - - /* Try again. */ - cpu_relax(); - return -EAGAIN; + /* + * This page might be a page granted by another domain, or this page + * is freed with decrease reservation hypercall at the same time. + */ + gdprintk(XENLOG_WARNING, + "bad page is passed. paddr 0x%lx maddr 0x%lx\n", + paddr, maddr); + return -EFAULT; } return 0; diff --git a/xen/include/asm-ia64/mm.h b/xen/include/asm-ia64/mm.h index 103628476c..9cadc21ba4 100644 --- a/xen/include/asm-ia64/mm.h +++ b/xen/include/asm-ia64/mm.h @@ -200,9 +200,7 @@ static inline void put_page(struct page_info *page) free_domheap_page(page); } -/* count_info and ownership are checked atomically. */ -static inline int get_page(struct page_info *page, - struct domain *domain) +static inline page_get_owner_and_reference(struct page_info *page) { unsigned long x, y = page->count_info; @@ -210,12 +208,21 @@ static inline int get_page(struct page_info *page, x = y; if (unlikely((x & PGC_count_mask) == 0) || /* Not allocated? */ unlikely(((x + 1) & PGC_count_mask) == 0) ) {/* Count overflow? */ - goto fail; + return NULL; } y = cmpxchg_acq(&page->count_info, x, x + 1); } while (unlikely(y != x)); - if (likely(page_get_owner(page) == domain)) + return page_get_owner(page); +} + +/* count_info and ownership are checked atomically. */ +static inline int get_page(struct page_info *page, + struct domain *domain) +{ + struct domain *owner = page_get_owner_and_reference(page); + + if (likely(owner == domain)) return 1; put_page(page); @@ -224,7 +231,7 @@ fail: gdprintk(XENLOG_INFO, "Error pfn %lx: rd=%p, od=%p, caf=%016lx, taf=%" PRtype_info "\n", page_to_mfn(page), domain, - page_get_owner(page), y, page->u.inuse.type_info); + owner, page->count_info, page->u.inuse.type_info); return 0; } diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index cde79e948e..7772e333cb 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -258,6 +258,7 @@ void cleanup_page_cacheattr(struct page_info *page); int is_iomem_page(unsigned long mfn); +struct domain *page_get_owner_and_reference(struct page_info *page); void put_page(struct page_info *page); int get_page(struct page_info *page, struct domain *domain); void put_page_type(struct page_info *page);